Implement e2e tests for export functionality#3506
Implement e2e tests for export functionality#3506DuBento wants to merge 31 commits intothought-machine:masterfrom
Conversation
build stmt checker now uses awk, supports more than one stmt associated with with a target and has a strict matching mode
Reverted changes to main test build def
toastwaffle
left a comment
toastwaffle
left a comment
There was a problem hiding this comment.
Let's ensure that each test repo has at least one target that does not get exported (getting it as close as possible to the edge of being exported). You've got quite a few cases here where the expected repo is identical to the test repo, which doesn't prove anything for the "export as little as possible" goal of exporting. Bonus points for making the e2e enforce that the test repo and expected repo are different.
Also, what's the behaviour around comments in BUILD files - is it worth a test case which exposes that?
| # Golden-Master validation with expected repo. Done before any building to avoid plz-out | ||
| f'diff -r "{exported_repo}" "$DATA_EXPECTED_REPO"', | ||
| # Tests building the exported target which in turn ensures the sources are included | ||
| f'plz {config_override} --repo_root="{exported_repo}" build {targets}', |
There was a problem hiding this comment.
Does it make sense for this to just be plz build //...? Each repo should be tiny, so building everything shouldn't be a problem, and it asserts our requirement that the entire export should be buildable?
|
|
||
| # Export a target generated by a native genrule target and trim unused | ||
| # build statements. | ||
| please_export_e2e_test( |
There was a problem hiding this comment.
Please could you move each of these please_export_e2e_test targets to a BUILD file alongside the test/expected repos?
| @@ -0,0 +1,5 @@ | |||
| filegroup( | |||
There was a problem hiding this comment.
Either this should contain an entry for dummy.build_defs, or dummy.build_defs shouldn't exist
| please_export_e2e_test( | ||
| name = "export_standard_children_custom_target", | ||
| cmd_on_export = [ | ||
| # Child of build def |
There was a problem hiding this comment.
Let's avoid referring to children, as that's not well-defined terminology. adjacent_targets_in_build_def is probably better naming for this case, and the fact that one is a "child" of the other doesn't make a difference.
What you're essentially capturing here is that a target A created in a build def alongside the target B that we're trying to export will result in both A and B being buildable
|
|
||
| # # Test go binary target. | ||
| please_export_e2e_test( | ||
| name = "export_go_binary", |
There was a problem hiding this comment.
Other than the target being exported moving to a subdirectory, I don't see any behavioural difference between this and export_go_target_with_go_dep
| ) | ||
|
|
||
| # Test outputs export. | ||
| please_repo_e2e_test( |
There was a problem hiding this comment.
Why do you not use please_export_e2e_test here? I think both would be clearer as golden-master tests
| ) | ||
|
|
||
| # Export a target from a repo that preloads a build def. | ||
| # This test purposely doesn't use the custom def but checks that the source files are still included. |
There was a problem hiding this comment.
| # This test purposely doesn't use the custom def but checks that the source files are still included. | |
| # The exported target doesn't use the preloaded build def, but the preloaded build def is still present in the export. |
| testing repos that include Go targets. | ||
| """ | ||
| EXPORT_DIR = "plz-out/plzexport" | ||
| test_repo = f"{dir}/test_repo" |
There was a problem hiding this comment.
nit: can we call this source_repo rather than test_repo throughout please
| config_override = "" | ||
| if include_go: | ||
| config_override += "-o plugin.go.gotool:$TOOLS_GO" | ||
| tools["GO"] = [CONFIG.GO.GO_TOOL] |
There was a problem hiding this comment.
is this necessary at all?
|
|
||
| config_override = "" | ||
| if include_go: | ||
| config_override += "-o plugin.go.gotool:$TOOLS_GO" |
There was a problem hiding this comment.
can't we just put this in the .plzconfig in the source repo?
Previously for every file we were looping through every target in the graph O(NxM). Now we first build a map of targets for each file for the whole graph then loop through the files to output the labels O(N+M). This vastly improves the performance of plz query what inputs for a large graph and large number of files.
I think this might be required before trying to upgrade the golang version in please repository, (thought-machine#3498) to enable CircleCI setup to run tests properly Co-authored-by: roselyn <roselyn@thoughtmachine.net>
…ne#3502) ``` str.ljust(width[, fillchar]) str.rjust(width[, fillchar]) ``` These methods are equivalent to their Python namesakes: they return a copy of the string left-padded or right-padded with the character given by `fillchar` until it is `width` characters long. `fillchar` defaults to `' '` (i.e. a space).
…#3504) `str.ljust` should left-justify `str` in the return value, and `str.rjust` should right-justify `str` in the return value - in other words, `str.ljust` should right-pad `str`, and `str.rjust` should left-pad `str`.
…machine#3507) The v1.1 endpoint (`/api/v1.1/project/github/...`) is deprecated. Switch to the v2 pipeline endpoint (`/api/v2/project/gh/.../pipeline`) which uses header-based auth and triggers the full pipeline.
We still have a glut of CircleCI credit to spend...
6 participants
This changes introduce small, targeted export test cases. The goal is to use these test for validating upcoming changes to the export logic.
please_export_e2e_testbuild def that is heavily inspired byplease_repo_e2e_testbut branches away to avoid injecting any base configuration files.test_repoand anexpected_repowith the entire files for testing and for validating the export.please_repo_e2e_testwere added to accommodate additional logic.